feat(pipes): add LogDestination implementation #31672

msambol · 2024-10-06T05:22:53Z

msambol · 2024-10-06T22:46:36Z

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

@@ -1,5 +1,9 @@
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { IDeliveryStream } from '@aws-cdk/aws-kinesisfirehose-alpha';


@paulhcsun I saw you working on this module. Can I use this here??

Hi @msambol, sorry I missed this. There were a couple of breaking changes added to the aws-kinesisfirehose-alpha module recently right before we moved it to Developer Preview. From a quick look I think it's fine in this use case but I'd recommend taking a look at the release notes to see if anyof the recent changes conflicts with the usage of the DeliveryStream in this PR.

Thanks, @paulhcsun! looks good from my view too. I'm just importing IDeliveryStream here.

aws-cdk-automation · 2024-10-30T00:12:33Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

nmussy

Had a quick look, I might have missed some things

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

nmussy · 2024-11-02T16:53:19Z

packages/@aws-cdk/aws-pipes-alpha/package.json

  },
  "dependencies": {},
  "peerDependencies": {
    "aws-cdk-lib": "^0.0.0",
-    "constructs": "^10.0.0"
+    "constructs": "^10.0.0",
+    "@aws-cdk/aws-kinesisfirehose-alpha": "0.0.0"


Is the peer dependency required here?

I think so.. because I am importing IDeliveryStream in logs.ts:

import { IDeliveryStream } from '@aws-cdk/aws-kinesisfirehose-alpha';

When I do npx lerna run build --scope=@aws-cdk/aws-pipes-alpha I get an error if I don't have it.

packages/@aws-cdk/aws-pipes-alpha/test/integ.pipe.ts

msambol · 2024-11-03T18:46:52Z

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

+      parameters: {
+        s3LogDestination: {
+          bucketName: this.parameters.bucket.bucketName,
+          bucketOwner: this.parameters.bucketOwner || this.parameters.bucket.env.account,


@nmussy I had to add this default otherwise I got the following:

Failed resources: aws-cdk-pipes | 10:24:59 AM | UPDATE_FAILED | AWS::Pipes::Pipe | Pipe (PipeXXX) Resource handler returned message: "[object has missing required properties (["BucketOwner"])] (Service: Pipes, Status Code: 400, Request ID: xxx)" (RequestToken: xxx, HandlerErrorCode: InvalidRequest)

Add this to the doc: @default - account ID derived from bucket

Shouldn't be an issue given how the env prop is built (see below), but just for safety could you add a bucket import in a unit test?

aws-cdk/packages/aws-cdk-lib/core/lib/resource.ts

Lines 171 to 174 in 5cb47da

this.env = {

account: props.account ?? parsedArn?.account ?? this.stack.account,

region: props.region ?? parsedArn?.region ?? this.stack.region,

};

Great shout. I added a unit test here: https://github.com/aws/aws-cdk/pull/31672/files#diff-7ee5baaeb407c0ce0a9d967e0d0ef9bd3523ede78b48d34fea55b538b030e739R564.

msambol · 2024-11-05T03:09:48Z

@paulhcsun Do you have time for a review here this week?

msambol · 2024-11-07T22:02:58Z

@Leo10Gama When you have time could you look here? This will be nice to round out the rest of pipes.

Leo10Gama

Thanks for the contribution! Seems like great work, I left a few comments on what could probably be improved and fine-tuned. It does seem like the original vision for this library was for the log destinations to be in their own folder like the targets and sources, but I'll double check with the CDK team to confirm if that's the route we want to go, since it may lead to more confusion later down the line.

packages/@aws-cdk/aws-pipes-alpha/README.md

Leo10Gama · 2024-11-14T23:08:01Z

packages/@aws-cdk/aws-pipes-alpha/README.md


-> Currently no implementation exist for any of the supported enrichments. The following example shows how an implementation can look like. The actual implementation is not part of this package and will be in a separate one.


It seems like the expected approach for this module is to have a new folder for the log destinations, likely akin to how we have aws-pipes-sources-alpha and aws-pipes-targets-alpha. We should probably follow this guideline, but I'll bring it up with the CDK team to confirm and get back to you, since it may add more confusion splitting one module into so many different parts.

Yeah, I'd prefer to leave these in aws-pipes but open to feedback from the team.

It seems like there is an argument to be made about having this be in the aws-pipes-alpha module, but there isn't really much extra cost having a separate aws-pipes-log-destinations-alpha folder, or something of that nature. One of the main reasons these are split the way they are is to prevent the risk of circular dependencies, and I'm not sure how big of a risk that could be in this case.

I think we could leave things as is, but I would lean more towards making a separate module for log destinations, and in the future before graduating this module, see if we can refactor things and consolidate everything into one. Thoughts?

I'd prefer to leave them here. Logging configuration is tied to a pipe, so I think it makes sense. But will of course default to you and the team.

Also, kinesis is now in dev preview which I think was the concern with circular dependency?

Not sure how dev preview might be a circular dependency concern; if anything I think the risk with that is, when it's out of dev preview and into GA, the import will need to be updated, but that's expected. It more so applies to the module as a whole, but we can keep the log destination resources here, and investigate refactoring at a later time.

Leo10Gama · 2024-11-14T23:09:27Z

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-pipes-pipe-s3logdestination.html#cfn-pipes-pipe-s3logdestination-bucketname
+   */
+  readonly bucket: IBucket;


The documentation you linked here shows that the bucket property isn't required, but we're mandating it here. I'm curious as to why that may be the case.

It's not required to create a pipe but it is required if you want to create an S3LogDestination. IMO the docs should be updated.

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

packages/@aws-cdk/aws-pipes-alpha/test/integ.pipe.ts

packages/@aws-cdk/aws-pipes-alpha/test/pipe.test.ts

Pull request has been modified.

msambol · 2024-11-17T20:12:58Z

Thanks for the contribution! Seems like great work, I left a few comments on what could probably be improved and fine-tuned. It does seem like the original vision for this library was for the log destinations to be in their own folder like the targets and sources, but I'll double check with the CDK team to confirm if that's the route we want to go, since it may lead to more confusion later down the line.

Update with your feedback – thanks!

Leo10Gama

Thanks for the timely response! Just a few more nits and minor comments on my end

packages/@aws-cdk/aws-pipes-alpha/README.md

packages/@aws-cdk/aws-pipes-alpha/test/integ.pipe.ts

Pull request has been modified.

codecov · 2024-11-19T03:49:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.46%. Comparing base (4db9565) to head (7c8c792).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #31672   +/-   ##
=======================================
  Coverage   77.46%   77.46%           
=======================================
  Files         105      105           
  Lines        7168     7168           
  Branches     1314     1314           
=======================================
  Hits         5553     5553           
  Misses       1433     1433           
  Partials      182      182

Flag	Coverage Δ
suite.unit	`77.46% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`77.46% <ø> (ø)`

msambol · 2024-11-19T04:10:02Z

Thanks for the timely response! Just a few more nits and minor comments on my end

Fixed! Thanks.

msambol · 2024-11-22T21:51:27Z

@Leo10Gama any further feedback on this one? Could we get this in too 😄

Leo10Gama

Thanks for your patience! Just one minor comment about the S3OutputFormat default value.

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts

Leo10Gama

Thanks for the quick response, changes look good to me!

Leo10Gama · 2024-11-26T23:56:59Z

@Mergifyio update

mergify · 2024-11-26T23:57:08Z

update

✅ Branch has been successfully updated

mergify · 2024-11-26T23:57:09Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-11-27T00:29:12Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-11-27T00:29:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 7c8c792
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

github-actions · 2024-11-27T00:29:32Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team October 6, 2024 05:23

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Oct 6, 2024

msambol force-pushed the pipes-logs branch from ef64746 to ab76855 Compare October 6, 2024 22:01

msambol marked this pull request as ready for review October 6, 2024 22:02

msambol commented Oct 6, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 7, 2024

msambol mentioned this pull request Oct 7, 2024

chore(pipes): update integration test, prune readme #31673

Closed

github-actions bot mentioned this pull request Nov 1, 2024

Monthly PR metrics report #31972

Closed

msambol force-pushed the pipes-logs branch from 6b1bf74 to 3bd6986 Compare November 1, 2024 23:09

nmussy suggested changes Nov 2, 2024

View reviewed changes

msambol force-pushed the pipes-logs branch from 5cb47da to f0e0064 Compare November 3, 2024 15:53

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 3, 2024

msambol commented Nov 3, 2024

View reviewed changes

Leo10Gama self-assigned this Nov 12, 2024

Leo10Gama previously requested changes Nov 14, 2024

View reviewed changes

msambol force-pushed the pipes-logs branch from 4255545 to 0aca775 Compare November 17, 2024 20:07

Leo10Gama previously requested changes Nov 18, 2024

View reviewed changes

packages/@aws-cdk/aws-pipes-alpha/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-pipes-alpha/test/integ.pipe.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-pipes-alpha/test/integ.pipe.ts Outdated Show resolved Hide resolved

msambol added 5 commits November 18, 2024 19:37

feat(pipes): add LogDestination implementation

e4232c0

Fix import order

11fe0b4

Trigger build

37db6cc

Fix readme

535f991

Fix readme

7ca4be6

msambol added 12 commits November 18, 2024 19:37

simplify integration test

7c92387

Update int test

87a4401

Fix int test

945ec65

Trigger build

b7a35be

Update docs

1638642

Add bucketOwner prop

ba57b9b

Throw error for TestTarget

b76f3fe

fix integration test

ce01f2d

Add unit test for imported bucket

f4d8f1a

Fix capitalization

0b890d7

Move tests to separate files, minimize permissions

a95dfb8

Update doc, add space

3c85b5a

msambol force-pushed the pipes-logs branch from 0aca775 to 3c85b5a Compare November 19, 2024 03:37

Leo10Gama reviewed Nov 26, 2024

View reviewed changes

packages/@aws-cdk/aws-pipes-alpha/lib/logs.ts Show resolved Hide resolved

Leo10Gama approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into pipes-logs

d1d05f5

Merge branch 'main' into pipes-logs

7c8c792

mergify bot merged commit af5345e into aws:main Nov 27, 2024
16 of 17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2024

msambol deleted the pipes-logs branch November 27, 2024 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pipes): add LogDestination implementation #31672

feat(pipes): add LogDestination implementation #31672

msambol commented Oct 6, 2024 •

edited

Loading

msambol Oct 6, 2024

paulhcsun Nov 6, 2024

msambol Nov 6, 2024

aws-cdk-automation commented Oct 30, 2024

nmussy left a comment

nmussy Nov 2, 2024

msambol Nov 3, 2024

msambol Nov 3, 2024 •

edited

Loading

nmussy Nov 4, 2024

msambol Nov 4, 2024

msambol commented Nov 5, 2024

msambol commented Nov 7, 2024

Leo10Gama left a comment

Leo10Gama Nov 14, 2024

msambol Nov 17, 2024

Leo10Gama Nov 17, 2024

msambol Nov 17, 2024 •

edited

Loading

msambol Nov 17, 2024

Leo10Gama Nov 18, 2024

Leo10Gama Nov 14, 2024

msambol Nov 17, 2024

msambol commented Nov 17, 2024

Leo10Gama left a comment

codecov bot commented Nov 19, 2024 •

edited

Loading

msambol commented Nov 19, 2024

msambol commented Nov 22, 2024

Leo10Gama left a comment

Leo10Gama left a comment

Leo10Gama commented Nov 26, 2024

mergify bot commented Nov 26, 2024

mergify bot commented Nov 26, 2024

mergify bot commented Nov 27, 2024

aws-cdk-automation commented Nov 27, 2024

github-actions bot commented Nov 27, 2024

		@@ -1,5 +1,9 @@
		// eslint-disable-next-line import/no-extraneous-dependencies
		import { IDeliveryStream } from '@aws-cdk/aws-kinesisfirehose-alpha';

	this.env = {
	account: props.account ?? parsedArn?.account ?? this.stack.account,
	region: props.region ?? parsedArn?.region ?? this.stack.region,
	};


		> Currently no implementation exist for any of the supported enrichments. The following example shows how an implementation can look like. The actual implementation is not part of this package and will be in a separate one.

feat(pipes): add LogDestination implementation #31672

feat(pipes): add LogDestination implementation #31672

Conversation

msambol commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 30, 2024

nmussy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msambol Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msambol commented Nov 5, 2024

msambol commented Nov 7, 2024

Leo10Gama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msambol Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msambol commented Nov 17, 2024

Leo10Gama left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

msambol commented Nov 19, 2024

msambol commented Nov 22, 2024

Leo10Gama left a comment

Choose a reason for hiding this comment

Leo10Gama left a comment

Choose a reason for hiding this comment

Leo10Gama commented Nov 26, 2024

mergify bot commented Nov 26, 2024

✅ Branch has been successfully updated

mergify bot commented Nov 26, 2024

mergify bot commented Nov 27, 2024

aws-cdk-automation commented Nov 27, 2024

AWS CodeBuild CI Report

github-actions bot commented Nov 27, 2024

msambol commented Oct 6, 2024 •

edited

Loading

msambol Nov 3, 2024 •

edited

Loading

msambol Nov 17, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading